SDK-less Google-SignIn – Part 2 of n #717

mokagio · 2022-12-08T11:40:43Z

Adds objects to model OAuth token request and response, the pre-requisite Proof of Code Encryption Key object for the request that generates the URL to use for the token, and the logic to generate the token request URL.

@crazytonyli one disadvantage of using an incremental approach to building a big feature such as this is that the first PRs fail to tell the story of the system being developed, because they are only individual components not yet used together.

Is this format acceptable for you to review? Otherwise I could:

Rework my WIP code to create a new PR that adds the higher level consumer of all these pieces (working name NewGoogleAuthenticator) so that we can do PRs that add a few bits and also show how they are used; or
Dump what I have in a >800 lines PR, after which it might be easier to move adding little bits to the existing end-to-end setup (eg. more error handling, MFA, etc...)
Keep working like this and share another maybe 2-3 PRs before we get to a working end-to-end bare bone setup

Let me know what would work best for you. Thanks!

To test

If CI is green, we're good.

+            // TODO: We might want to add some of these or them configurable
+            //
+            // The request we make with the SDK asks for:
+            //
+            // - email
+            // - profile
+            // - https://www.googleapis.com/auth/userinfo.email
+            // - https://www.googleapis.com/auth/userinfo.profile
+            // - openid
+            //
+            // See https://developers.google.com/identity/protocols/oauth2/scopes
+            ("scope", "https://www.googleapis.com/auth/userinfo.email")


Spoiler: I updated this in my WIP branch but haven't seen any difference in the behavior, but that's likely due to my implementation being quite minimal at this point. Or, it's possible that only the backend would notice the difference, when completing the authentication using the OAuth token.

mokagio · 2022-12-08T11:47:56Z

WordPressAuthenticator/GoogleSignIn/URL+GoogleSignIn.swift

+            guard var components = URLComponents(url: baseURL, resolvingAgainstBaseURL: false) else {
+                throw URLError(
+                    .unsupportedURL,
+                    userInfo: [
+                        NSLocalizedDescriptionKey: "Could not create `URLComponents` instance from \(baseURL)"
+                    ]
+                )
+            }


Do you think this is acceptable as a URLError, or should I update it to another OAuthError case for consistency?

I'm leaning toward moving it to OAuthError.

I think it's safe to ! here, since we already ! the googleSignInBaseURL?

That's a great observation! After all, what are the chances URLComponents will not initialize with a valid input URL?

Addressed in d749c80

mokagio · 2022-12-08T11:49:54Z

WordPressAuthenticator/OAuth/OAuthTokenResponseBody.swift

+    /// The type of token returned. At this time, this field's value is always set to Bearer.
+    let tokenType: String


The documentation comment I lifted from from the Google docs makes me wonder whether there's even a point in storing it at this time?

mokagio · 2022-12-08T11:50:50Z

WordPressAuthenticator/OAuth/ProofKeyForCodeExchange.swift

+//
+// A code_verifier is a high-entropy cryptographic random string using the unreserved
+// characters [A-Z] / [a-z] / [0-9] / "-" / "." / "_" / "~", with a minimum length of 43
+// characters and a maximum length of 128 characters.


I plan to address the completion of this type in a dedicated PR. The process works for development purposes in the current, plain mode.

mokagio · 2022-12-08T11:52:00Z

WordPressAuthenticatorTests/OAuth/ProofKeyForCodeExchangeTests.swift

+    func testCodeChallengeInS256ModeIsEncodedAsPerSpec() {
+        // TODO:
+    }


I put this down as a double reminder to finish the PKCE implementation. See also previous comment.

crazytonyli · 2022-12-12T06:37:37Z

@mokagio I'm okay with how ever you'd like to break down the end-to-end flow. I don't see any difficulties reviewing individual component, low-level or UI. 😄

crazytonyli · 2022-12-12T06:44:45Z

WordPressAuthenticator/GoogleSignIn/URL+GoogleSignIn.swift

+    // error, which we would catch because the unit tests would crash.
+    static var googleSignInBaseURL = URL(string: "https://accounts.google.com/o/oauth2/v2/auth")!

+    static func googleSignInAuthURL(clientId: String, pkce: ProofKeyForCodeExchange) throws -> URL {


Since we are essentially implement OAuth 2, I think we can replace "Google Sign In" with a more generic term? Like, this function becomes oauth2AuthenticationURL, what do you think?

I'm making assumption here, but I imagine the whole Google Sign In workflow can be used in "Sign In via Facebook", or even the WordPress OAuth2.

Good point. I was thinking about this before going AFK and realized I may be using unclear boundaries here.

You might have noticed I made an OAuth folder and a GoogleSignIn one. My intent was to separate the components that are GoogleSignIn specific from those that are part of the OAuth spec. One issue with how I executed this is that I haven't been cross checking between the OAuth and Google documentations so there might be stuff in the OAuth folder that's still Google specific, and vice versa.

Thinking about it since, I came to the conclusion that I'd prefer to separate OAuth- from Google- specific code only once we'll need to add a new OAuth client (likely WordPress OAuth2). As such, I'm tempted to follow up this PR with one that moves all the OAuth/ code into GoogleSignIn/, renaming some of the types if necessary.

My rationale for the suggestion above is that I'm more interested in having a working version of the SDK-less authentication than a general purpose OAuth client, even though building a client first might make it easier to add further authentication implementation in the future.

What do you think?

Sounds good to me. We probably don't need to "pre-abstract" stuff if there is no immediate plan to support another OAuth provider (service? server?). I trust you'll strike the right balance 👍

I trust you'll strike the right balance 👍

Thanks for the confidence.

crazytonyli · 2022-12-12T06:48:28Z

WordPressAuthenticator/GoogleSignIn/URL+GoogleSignIn.swift

+            guard var components = URLComponents(url: baseURL, resolvingAgainstBaseURL: false) else {
+                throw URLError(
+                    .unsupportedURL,
+                    userInfo: [
+                        NSLocalizedDescriptionKey: "Could not create `URLComponents` instance from \(baseURL)"
+                    ]
+                )
+            }


I think it's safe to ! here, since we already ! the googleSignInBaseURL?

crazytonyli · 2022-12-12T06:49:47Z

WordPressAuthenticator/GoogleSignIn/URL+GoogleSignIn.swift

+            }
+
            components.queryItems = queryItems
            return try components.asURL()


It's alsy safe to ! here too?

Also, this asURL is an Alamofire function, maybe we shouldn't use it here since it throws an AFError?

Another good point. Addressed in d5c6fd0

crazytonyli · 2022-12-12T07:29:34Z

WordPressAuthenticator/OAuth/OAuthTokenRequestBody.swift

+        var components = URLComponents()
+        components.queryItems = items
+
+        guard let query = components.query else {


I don't think it's possible to have a nil query in this case?

Yes. That should be the case.

🤔 I guess I'm just uncomfortable force unwrapping stuff, but it does make for simpler code in a case like this one.

I tried to write a test that resulted in this code throwing, but I wasn't able to. That's not to say that there is no way for the failure to occur, of course. Still, I feel a bit more confident about removing the errors here.

Addressed in 8b6f6a8

crazytonyli · 2022-12-12T07:30:50Z

WordPressAuthenticator/OAuth/OAuthTokenRequestBody.swift

+            throw OAuthError.failedToBuildURLQuery
+        }
+
+        guard let data = query.data(using: .utf8) else {


I don't think it's possible to fail to utf8 encode the query string?

Addressed in 8b6f6a8 while working on your previous comment.

Thanks!

crazytonyli · 2022-12-12T07:34:40Z

WordPressAuthenticator/OAuth/ProofKeyForCodeExchange.swift

+// The code verifier should have enough entropy to make it impractical to guess the value.
+struct ProofKeyForCodeExchange {
+
+    enum Mode {


I'm not good at naming things, but what do you think about renaming this to Method and the var method to something like var googleOAuthParameterName?

Thank you for raising this. Method would definitely be a better name, as that's what the spec uses 👍

Addressed in 8abbb29.

As for the computed variable name, I went for urlQueryParameterValue to try and make it clearer where to use it.

crazytonyli · 2022-12-12T07:35:35Z

WordPressAuthenticator/OAuth/ProofKeyForCodeExchange.swift

+    let codeVerifier: String
+    let mode: Mode
+
+    init(codeVerifier: String, mode: Mode) {


Considering we should randomly generate codeVerifier, do you think we can make this argument nil by default and generate a random string internally?

Thank you for the suggestion. I'm keen on generating the string internally according to the spec but I think that's work that could live in a dedicated PR. Does that sound okay?

What I have in mind is along the lines of your suggestion, with the difference of providing the random string as the default value.

init(codeVerifier: String = makeRandomPKCEString(), method: Method = .s256) {

See also discussion at https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/pull/717/files#r1043265177

This also resulted in renaming the computed variable `method` to `testMethodURLQueryParameterValue` because `Method.method` would have been confusing. The new name also better reflects where the value is meant to be used.

See discussion at https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/pull/717/files#r1045479156

mokagio added 12 commits December 8, 2022 10:02

Add object modelling Proof Key For Code Exchange

f15815f

Add computation of code_challenge_method

9c8b1d6

Add PKCE parameter to GoogleSignIn URL

7f93ab0

Add scope parameter to GoogleSignIn URL

7c8b61c

Add FIXME note regarding GoogleSignIn URL error handling

6b1db97

Extract base URL for first step of GoogleSignIn flow and force unwrap it

ccdfc0b

Throw error in iOS <= 15.0 branch if URLComponents fails to init

b20fbe7

Add object modelling request body for OAuth token

5c5e160

Throw errors from asURLEncodedData()

d5bdf02

Add object modelling OAuth token response

93c78bc

Add method to create preconfigured URLRequest for OAuth token

6e34f3a

Add method to init OAuthRequestBody configured for GoogleSignIn

1798c5a

mokagio commented Dec 8, 2022

View reviewed changes

WordPressAuthenticator/GoogleSignIn/OAuthRequestBody+GoogleSignIn.swift Outdated Show resolved Hide resolved

Remove an outdated TODO note

39efd42

mokagio commented Dec 8, 2022

View reviewed changes

WordPressAuthenticator/GoogleSignIn/URL+GoogleSignIn.swift Outdated Show resolved Hide resolved

Remove another outdated TODO note

bccc9c3

mokagio commented Dec 8, 2022

View reviewed changes

crazytonyli reviewed Dec 12, 2022

View reviewed changes

Simplify googleSignInAuthURL method

d749c80

See also discussion at https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/pull/717/files#r1043265177

mokagio force-pushed the mokagio/oauth-part-2-of-n branch from 63a01aa to d749c80 Compare January 5, 2023 02:46

mokagio added 4 commits January 5, 2023 13:48

Do not use Alamofire code in URL+GoogleSignIn.swift

d5c6fd0

Add additional documentation references to PKCE type

e963a39

Rename PKCE Mode to Method to be in line with spec

8abbb29

This also resulted in renaming the computed variable `method` to `testMethodURLQueryParameterValue` because `Method.method` would have been confusing. The new name also better reflects where the value is meant to be used.

Remove redundant error handling from asURLEncodedData

8b6f6a8

See discussion at https://github.com/wordpress-mobile/WordPressAuthenticator-iOS/pull/717/files#r1045479156

mokagio requested a review from crazytonyli January 5, 2023 03:45

crazytonyli approved these changes Jan 8, 2023

View reviewed changes

mokagio marked this pull request as ready for review January 18, 2023 03:40

mokagio merged commit d170cbd into trunk Jan 18, 2023

mokagio deleted the mokagio/oauth-part-2-of-n branch January 18, 2023 03:41

mokagio mentioned this pull request Jan 25, 2023

SDK-less GoogleSignIn - Part 3 #728

Merged

1 task

		/// The type of token returned. At this time, this field's value is always set to Bearer.
		let tokenType: String

SDK-less Google-SignIn – Part 2 of n #717

SDK-less Google-SignIn – Part 2 of n #717

Uh oh!

Conversation

mokagio commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To test

See also

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mokagio Jan 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crazytonyli commented Dec 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mokagio commented Dec 8, 2022 •

edited

Loading

mokagio Jan 5, 2023 •

edited

Loading